Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): improved error objects #1217

Merged
merged 5 commits into from
Sep 23, 2024
Merged

feat(api): improved error objects #1217

merged 5 commits into from
Sep 23, 2024

Conversation

dohaki
Copy link
Contributor

@dohaki dohaki commented Sep 23, 2024

Closes ACX-2705

This PR proposes improved error objects returned by the API. The goal is to have better DX and debugging possibilities.

Copy link

linear bot commented Sep 23, 2024

Copy link

vercel bot commented Sep 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app-frontend-v3 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 23, 2024 2:59pm
sepolia-frontend-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 23, 2024 2:59pm

Copy link

@bmzig bmzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

throw new InputError("Sent amount is too low relative to fees");
throw new AmountTooLowError({
message: `Sent amount is too low relative to fees. Minimum deposit is ${ethers.utils.formatUnits(
minDeposit,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a UX quirk on sharing minDeposit: it's set predominantly based on destination gas, so it's subject to fluctuate slightly between queries. If a caller gets this error response and then resubmits w/ minDeposit there's a decent chance that it will fail again, and they'll need to set another minDeposit.

idk whether we can (or even should) do anything about that in this context, but it's worth thinking about for a follow-up change. The only thing I can think of is it round minDeposit up to some round number to give some padding. That's an approach with its own flaws, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yea that's a good point. I will remove the value from he error then for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants